-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement API tagging location #2847
Conversation
Hey, @Gauravano @sagarpreet-chadha @jywarren could you please take a look here? I've opened this PR to gather some feedback, what do you think? Thanks! |
@publiclab/reviewers please review :) I'll fix the codeclimate issues tomorrow. |
Generated by 🚫 Danger |
Looking great!!! @sagarpreet-chadha what do you think of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefannibrasil ...the changes are great !
coordinates = srchString.split(",") | ||
lat = coordinates[0] | ||
lon = coordinates[1] | ||
lat, lon = srchString.split(',') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing 😄 !
@stefannibrasil ...can you post a screenshot of the the JSON ? |
app/services/search_service.rb
Outdated
lon = coordinates[1] | ||
lat, lon = srchString.split(',') | ||
|
||
tagList = textSearch_tags(tagName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stefannibrasil !
This looks great !
Just one thing --- should we not show nodes with tagName which our near the given Latitude and Longitude .
This will add all nodes having tag = tagName , right ?
What do you think ?
AND @stefannibrasil ...if a tagname is given then we need to show - nodes near given LAT and LON value having tag = tagname . Else if , tagname is not given in URL then we need to show all near by nodes . @jywarren ...can you confirm this ? |
Yep, that's right! |
Right, I'll work on this, makes sense. Thanks, I'll let you know when I have it changed, cheers! |
Everything going well here? Just checking, thanks! |
HI, @jywarren sorry, I am a little late with this one. I will work on this tomorrow, I am getting some things done before RGSoC starts, that's why I couldn't finish this. Thanks for checking in! =) |
No problem at all, and no rush! Just checking if you were stuck and
frustrated with something we could help with. Thanks!
…On Fri, Jun 29, 2018, 3:43 PM Stefanni ***@***.***> wrote:
HI, @jywarren <https://github.com/jywarren> sorry, I am a little late
with this one.
I will work on this tomorrow, I am getting some things done before RGSoC
starts, that's why I couldn't finish this. Thanks for checking in! =)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2847 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ_VWKEFKq_aTJIBl-IuVkWQrHryeks5uBoNNgaJpZM4UsqRU>
.
|
hi, @jywarren @sagarpreet-chadha I made some changes, could you take a look to see if that's what we need, please? |
app/api/srch/search.rb
Outdated
sresult = DocList.new | ||
unless params[:srchString].nil? || params[:srchString] == 0 || !(params[:srchString].include? ",") | ||
sservice = SearchService.new | ||
sresult = sservice.nearbyNodes(params[:srchString]) | ||
sresult = sservice.tagNearbyNodes(params[:srchString], params[:tag]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params[:tag] should be params[:tagName] , right ?
app/services/search_service.rb
Outdated
|
||
nids ||= [] | ||
if tagName.present? | ||
nodes_scope = nodes_scope.where('name LIKE ?', tagName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 💯 !
Hello @jywarren @sagarpreet-chadha, Stefanni and I made more changes. When possible, could you take a look? Thanks! |
This looks awesome and I love the tests. Great work!!! 🎉 👍 👍 |
* API tagging locations implementation * Requested changes for the API tagging implementation * Requested changes for the API tagging implementation
Improve API location search (#1934 ) to also include tags on the request URL.
Fix #2790 and #2254
rake test
fixes #0000
-style reference to original issue #@publiclab/reviewers
for help, in a comment below